feat: implement Prompt.save() with automatic dirty tracking for name changes #504
feat: implement Prompt.save() with automatic dirty tracking for name changes #504thiagobomfin-galileo wants to merge 1 commit intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #504 +/- ##
==========================================
+ Coverage 82.08% 82.39% +0.31%
==========================================
Files 96 103 +7
Lines 9297 9426 +129
==========================================
+ Hits 7631 7767 +136
+ Misses 1666 1659 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def __setattr__(self, attr_name: str, value: Any) -> None: | ||
| """Track mutations to name and transition to DIRTY state when synced.""" | ||
| if attr_name in self._TRACKED_FIELDS and hasattr(self, "_sync_state") and self._sync_state == SyncState.SYNCED: | ||
| self._set_state(SyncState.DIRTY) | ||
| object.__setattr__(self, attr_name, value) | ||
|
|
There was a problem hiding this comment.
Prompt.setattr flips a synced object's state to DIRTY on any assignment to tracked fields without checking equality, so prompt.name = prompt.name becomes DIRTY and save() (lines 775–788) then calls update(name=self.name), issuing an unnecessary API rename. Can we compare the new value to the current one before flipping the sync state so redundant assignments don't trigger updates?
Finding type: Logical Bugs | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In src/galileo/__future__/prompt.py around lines 209-214, the __setattr__ method
unconditionally marks tracked fields DIRTY when synced. Change it to first obtain the
current value via object.__getattribute__(self, attr_name) (or use a unique sentinel
with getattr) and compare it to the new value; only call
self._set_state(SyncState.DIRTY) if the values differ. Ensure this comparison handles
the missing-attribute case safely (treat missing as different) and then delegate to
object.__setattr__ to assign the value.
There was a problem hiding this comment.
Commit b1e89cf addressed this comment by checking the current tracked attribute value and only marking the prompt DIRTY if the new value differs, preventing redundant rename updates.
| """ | ||
| self.id = retrieved_prompt.id | ||
| self.name = retrieved_prompt.name | ||
| object.__setattr__(self, "name", retrieved_prompt.name) | ||
|
|
There was a problem hiding this comment.
What's wrong: Both _update_from_api_response and update() now manually call object.__setattr__(self, "name", …) to bypass the new dirty-tracking override when syncing the name. The same three-line pattern appears twice (lines 301‑304 and again around 506‑511), so any future tracked field or additional sync flow will need the same boilerplate.
Impact: We risk forgetting to bypass the overridden __setattr__ in new code paths, which would incorrectly mark the prompt as DIRTY and block save()/update() operations or cause redundant API calls.
Ask: Can we extract a helper (e.g., _set_synced_name(self, name: str) or a generic _assign_tracked_attr(attr, value)) and call it from both _update_from_api_response and update() so the bypass logic lives in one place and isn’t duplicated?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
- Analysis: There are only two call sites for
object.__setattr__(self, "name", ...). A helper for two occurrences is premature abstraction. The pattern is clear and self-documenting with the inline comment. If more tracked fields are added later, a helper would make sense at that point. - Recommended action: Leave as-is. Two call sites don't warrant an abstraction.
There was a problem hiding this comment.
Got it, I’ll leave this pattern as-is and will remember your preference against premature abstraction here once the PR is merged.
src/galileo/__future__/prompt.py
Outdated
| # DIRTY or FAILED_SYNC: name was changed, persist it | ||
| if self.id is None: | ||
| raise ValueError("Prompt ID is not set. Cannot update a prompt without an ID.") | ||
| return self.update(name=self.name) |
There was a problem hiding this comment.
Prompt.save() treats any FAILED_SYNC as a name-only dirty state and unconditionally calls self.update(name=self.name) (e.g. after a failed create_version). This can hide the original failure and leave the prompt incorrectly marked SYNCED even though the failed operation never completed. Can we restrict save() to retry only name-related DIRTY changes or record/distinguish the failure source before invoking self.update(name=...)?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In src/galileo/__future__/prompt.py around lines 785 to 788, the save() method currently
treats FAILED_SYNC the same as DIRTY and unconditionally calls update(name=self.name),
which can mask unrelated failures. Change this logic so that only SyncState.DIRTY
triggers the update retry: if self.sync_state == SyncState.DIRTY, validate self.id and
call return self.update(name=self.name); if self.sync_state == SyncState.FAILED_SYNC, do
not call update — instead re-raise or surface the stored error (e.g., raise the saved
error from the failed sync state or raise a ValueError instructing the caller to retry
the original operation), preserving the original failure information and avoiding
marking the prompt as SYNCED incorrectly.
There was a problem hiding this comment.
Commit b1e89cf addressed this comment by short-circuiting FAILED_SYNC before any update retry and raising a clear ValueError directing callers to resync or retry, ensuring only genuine DIRTY states call update(name=self.name).
test(future): add TestPromptSave covering dirty tracking and save() dispatch
b1e89cf to
c4733a9
Compare
| raise | ||
|
|
||
| def save(self) -> Prompt: | ||
| """ |
There was a problem hiding this comment.
- [Minor]:
save()docstring is stale — it doesn't document the DIRTY or FAILED_SYNC behavior.- File:
src/galileo/__future__/prompt.py(lines 756-779) - The docstring says "Behavior depends on the prompt's current state" but only lists LOCAL_ONLY, SYNCED, and DELETED. It should also document DIRTY (delegates to
update()) and FAILED_SYNC (raisesValueError). - Suggestion: Update the docstring to include:
- DIRTY: Persists name changes via update() - FAILED_SYNC: Raises ValueError (use refresh() to re-sync)
- File:
|
|
||
| def __setattr__(self, attr_name: str, value: Any) -> None: | ||
| """Track mutations to name and transition to DIRTY state when synced.""" | ||
| if attr_name in self._TRACKED_FIELDS and hasattr(self, "_sync_state") and self._sync_state == SyncState.SYNCED: |
There was a problem hiding this comment.
- [Nit]:
_TRACKED_FIELDSis annotated as a class variable withfrozenset[str]but it's accessed viaself._TRACKED_FIELDSin__setattr__. This works via Python's descriptor protocol, but usingtype(self)._TRACKED_FIELDSorPrompt._TRACKED_FIELDSwould make the intent clearer and avoid potential shadowing if an instance ever sets_TRACKED_FIELDSas an instance attribute.- File:
src/galileo/__future__/prompt.py(line 211)
- File:
| version_info = f", version={self.selected_version_number}" if self.selected_version_number else "" | ||
| return f"Prompt(name='{self.name}', id='{self.id}', messages={len(self.messages)} messages{version_info})" | ||
|
|
||
| def __setattr__(self, attr_name: str, value: Any) -> None: |
There was a problem hiding this comment.
- Is there a plan to apply the same
__setattr__dirty-tracking pattern to other__future__domain objects (Project, Dataset)? If so, it might be worth extracting the__setattr__logic into theStateManagementMixinbase class at that point. Not needed now, but worth considering for consistency across the package.
User description
Shortcut:
Implement Missing CRUD Operations in future Package
Description:
object automatically transitions its state to DIRTY
by delegating to the existing update() method
from an API response, bypassing dirty tracking to avoid false DIRTY transitions during internal
syncs
logic, error handling, and state transitions are already in update()
Design decision: name only
Prompt has two mutable attributes that map to different API endpoints:
Only name is tracked for dirty state. Changing messages requires an explicit create_version() call — making version creation an intentional action rather than a silent side effect of save().
Tests:
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Track name mutations on
Promptso settingprompt.nameflips a synced object to DIRTY while internal syncs bypass dirty tracking. Delegate the DIRTY/FAILED_SYNC branch ofPrompt.save()toupdate()with ID and state guards while covering the behavior with new unit tests.Promptname changes so assignments on synced objects mark them DIRTY and makesave()callupdate()with ID/state guards while bypassing dirty tracking during internal syncs.Modified files (1)
Latest Contributors(2)
save()guards through tests that exercise the DIRTY branch, ID validation, failure handling, and refresh behavior.Modified files (1)
Latest Contributors(1)